-
Notifications
You must be signed in to change notification settings - Fork 741
[aoti-backend-consolidation 2/3] backend.py #15528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/15528
Note: Links to docs will display an error until the docs builds have been completed. ❌ 4 New Failures, 1 Cancelled JobAs of commit 57e8ffe with merge base d2c011e ( NEW FAILURES - The following jobs have failed:
CANCELLED JOB - The following job was cancelled. Please retry:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@Gasoonjia has exported this pull request. If you are a Meta employee, you can view the originating Diff in D85704977. |
larryliu0820
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review automatically exported from Phabricator review in Meta.
This PR needs a
|
| blob_data = f.read() | ||
|
|
||
| named_data_store.add_named_data( | ||
| method_name + "_weights_blob", blob_data, 1, "aoti_metal_blob" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the new name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's the new name: https://github.com/pytorch/executorch/pull/15528/files#r2496118034
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh i called it mps now instead of metal; thanks let me solve it
| named_data_store.add_named_data(method_name + "_so_blob", so_data, 1, None) | ||
| weights_blob_data_type = f"aoti_{device_name}_blob" | ||
| named_data_store.add_named_data( | ||
| method_name + "_weights_blob", blob_data, 1, weights_blob_data_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's the blob name in the new setting @manuelcandales
|
|
||
| # Add SO and weights blob separately | ||
| named_data_store.add_named_data(method_name + "_so_blob", so_data, 1, None) | ||
| weights_blob_data_type = f"aoti_{device_name}_blob" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this will now be aoti_mps_blob, so, need to update this in metal.yml (uses aoti_metal_blob)
| subclasses.update(_get_all_final_backend_details_subclasses(subclass)) | ||
| return subclasses | ||
|
|
||
| backend_name_to_subclass = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember it was discussed at the beginning of the delegate meeting - the backend implementation should be final and we didn't want to get involve in nested backends
executorch/exir/backend/backend_api.py
Line 111 in 3e9629a
| # All backend implementation are final, so we don't need to consider nested subclasses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the example we put, we did try to put a final
| @final |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are shared logic, maybe put up a shared folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for comments. I make the AOTIBackend as a collector of sharing logics but not a actual backend, and make CUDABackend and MetalBackend inherit from both AOTIBackend and BackendDetails to avoid making any update on backend_api.py.
| @experimental( | ||
| "This API and all of aoti-driven backend related functionality are experimental." | ||
| ) | ||
| class AotiBackend(ABC): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is AotiBackend an actual class or an abstract class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading, it seems like we will just have metal backend and cuda backend as the actual backend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it is just a abstract class not a real backend for meet backend_details' requirement
setup.py
Outdated
|
|
||
| if is_linux_x86(): | ||
| os.environ["EXECUTORCH_BUILDING_WHEEL"] = "1" | ||
| from backends.qualcomm.scripts.download_qnn_sdk import _download_qnn_sdk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mengwei has this PR #15546 maybe can coordinate how to land together or maybe after he merged his change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have reverted my changes
Summary: pytorch#15528 initially wanted to subclass a backend.. It was currently already guarded by https://github.com/pytorch/executorch/blob/main/exir/backend/backend_api.py#L111-L112 meaning that subclass will not show up. However it's not super obvious so we want to guard by disallowing subclass at all Differential Revision: D87105211
Summary: pytorch#15528 initially wanted to subclass a backend.. It was currently already guarded by https://github.com/pytorch/executorch/blob/main/exir/backend/backend_api.py#L111-L112 meaning that subclass will not show up. However it's not super obvious so we want to guard by disallowing subclass at all Reviewed By: Gasoonjia Differential Revision: D87105211
Summary: #15528 initially wanted to subclass a backend.. It was currently already guarded by https://github.com/pytorch/executorch/blob/main/exir/backend/backend_api.py#L111-L112 meaning that subclass will not show up. However it's not super obvious so we want to guard by disallowing subclass at all Differential Revision: D87105211
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR consolidates AOTI (AOT Inductor) backend functionality by creating a shared base class (AotiBackend) that can be reused across different device backends (CUDA, Metal). The refactoring reduces code duplication and standardizes the compilation workflow.
Key Changes:
- Created a new
AotiBackendmixin class that provides common AOTI compilation logic - Refactored CUDA and Metal backends to inherit from
AotiBackendinstead of duplicating code - Updated build targets to reference the consolidated backend dependency
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backends/aoti/aoti_backend.py | New base class providing common AOTI compilation functionality |
| backends/cuda/cuda_backend.py | Simplified to use AotiBackend with CUDA-specific configuration |
| backends/apple/metal/metal_backend.py | Simplified to use AotiBackend with Metal-specific configuration |
| backends/aoti/targets.bzl | Added build target for the new aoti_backend module |
| backends/cuda/TARGETS | Updated to depend on consolidated aoti_backend target |
| setup.py | Moved platform detection function to avoid import errors |
| examples/models/whisper/CMakeLists.txt | Updated target name from aoti_cuda to aoti_cuda_backend |
| examples/models/gemma3/CMakeLists.txt | Updated target name from aoti_cuda to aoti_cuda_backend |
| extension/llm/tokenizers | Updated subproject commit reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @staticmethod | ||
| def get_supported_fallback_kernels() -> Dict[str, Any]: | ||
| return { | ||
| "aoti_torch_mps_addmm_out": None, |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kernel 'aoti_torch_mps_addmm_out' was added to the supported fallback kernels but was not present in the original code. Verify this kernel is intentionally supported and not an accidental addition during refactoring.
| "aoti_torch_mps_addmm_out": None, |
Summary: This diff consolidates the backend functionality into a single target `//executorch/backends/aoti:aoti_backend` and simplifies the cuda backend target by making it dependent on the consolidated backend target. The following changes are made in this diff: * Creation of a new target `//executorch/backends/aoti:aoti_backend` in `fbcode/executorch/backends/aoti/targets.bzl` which includes the necessary dependencies for the AOTI backend. * Update of the `//executorch/backends/cuda:cuda_backend` target in `fbcode/executorch/backends/cuda/TARGETS` to depend on the new `//executorch/backends/aoti:aoti_backend` target instead of individual AOTI backend dependencies. * Creation of a new file `fbcode/executorch/backends/aoti/aoti_backend.py` which imports the necessary dependencies and passes for the AOTI backend. * Simplification of the `xplat/executorch/backends/cuda/cuda_backend.py` file by removing unnecessary imports and using the new `AotiBackend` class from the `aoti_backend.py` file. ghstack-source-id: 319556735 Reviewed By: larryliu0820 Differential Revision: D85704977
d129e1e to
1e90fd0
Compare
|
@Gasoonjia has imported this pull request. If you are a Meta employee, you can view this in D85704977. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary:
Summary
This diff consolidates the backend functionality into a single target
//executorch/backends/aoti:aoti_backendand simplifies the cuda backend target by making it dependent on the consolidated backend target.The following changes are made in this diff:
//executorch/backends/aoti:aoti_backendinfbcode/executorch/backends/aoti/targets.bzlwhich includes the necessary dependencies for the AOTI backend.//executorch/backends/cuda:cuda_backendtarget infbcode/executorch/backends/cuda/TARGETSto depend on the new//executorch/backends/aoti:aoti_backendtarget instead of individual AOTI backend dependencies.fbcode/executorch/backends/aoti/aoti_backend.pywhich imports the necessary dependencies and passes for the AOTI backend.xplat/executorch/backends/cuda/cuda_backend.pyfile by removing unnecessary imports and using the newAotiBackendclass from theaoti_backend.pyfile.ghstack-source-id: 319556735
Reviewed By: larryliu0820
Differential Revision: D85704977